Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stats: Fix Stats view stuck on Insights tab if the grow audience card is showing #19417

Merged
merged 2 commits into from
Oct 10, 2022
Merged

Stats: Fix Stats view stuck on Insights tab if the grow audience card is showing #19417

merged 2 commits into from
Oct 10, 2022

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Oct 7, 2022

Description

Fixes an issue, when the stats view is stuck on the Insights tab and switching tabs, doesn't work.

Root cause

The issue is caused by Stay on insights tab if Grow Audience card is showing change.

  • When isGrowAudienceShowing is true we don't save the selected tab to user defaults.
  • However, it fails in cases when we already have the selected tab saved. updatePeriodView incorrectly assumes that the wrong tab is selected and doesn't set periodTableViewController in pageViewController.

Solution

When isGrowAudienceShowing is true, explicitly set selected tab to insights.

Testing instructions

Case 1:

  1. Fresh install app
  2. Open Stats
  3. Notice "A tip to grow your audience" is shown
  4. Dismiss this tip
  5. Select any of Days/Weeks/Months/Years
  6. Come back to Menu
  7. Select Stats again
  8. Previously selected Days/Weeks/Months/Years tab should open
  9. Open Insights
  10. Notice another "A tip to grow your audience" is shown
  11. Select any of Days/Weeks/Months/Years
  12. The tabs should be switching successfully

Regression Notes

  1. Potential unintended areas of impact

Stay on insights tab if Grow Audience card is showing

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

Manual testing scenarios

  1. What automated tests I added (or what prevented me from doing so)

None

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Images & Videos

Before

before.MP4

After

after.MP4

@staskus
Explicitly set insights as selected period when grow audience is showing
6184a5c 
When we simply return, we can get into cases where selectedPeriod is not equal to .insights even though insights are open. In such an instance the tab switching doesn't work anymore.
@staskus staskus added this to the 20.9 ❄️ milestone Oct 7, 2022
@staskus staskus requested review from guarani and twstokes October 7, 2022 14:39
@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@staskus
Copy link
Contributor Author

staskus commented Oct 7, 2022

Also cherry-picked bundler fix #19409

@wpmobilebot
Copy link
Contributor

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19417-25f69c9 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19417-25f69c9 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

guarani
guarani approved these changes Oct 10, 2022
View reviewed changes
Copy link
Contributor

@guarani guarani left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding out how to reproduce this bug and for fixing it! 🙇

These are the steps I used to reproduce (taken from p5T066-3CX-p2#comment-13674) and my understanding of the issue:

  1. Open Stats
  2. Dismiss the "A tip to grow your audience" tip card that's shown
  3. Switch to the Months tab (the tab selection is persisted)
  4. Close Stats
  5. Open Stats and the Months tab is shown (correct)
  6. Switch to the Insights tab and a different tip is shown
  • The new tab selection wasn't being persisted here, but with this PR it's correctly set to Insights
  1. Try to switch to the Month tab and notice the contents of the Insights tab are still show
  • The "old" and "new" tab selections are both "Months", so the logic here doesn't set the view controller to show the screen that corresponds to Months

I agree that the approach you've taken which persists the tab selection as Insights if there is a tip card to be shown – or the specific Day/Week/Year tab if there is no tip card – is a good approach. The logic to take the user to the Insights tab if there is a tip card available (#17332) – no matter what tab they were previously on – is still working.

guarani
guarani reviewed Oct 10, 2022
View reviewed changes
Gemfile.lock Show resolved Hide resolved
@staskus
Copy link
Contributor Author

staskus commented Oct 10, 2022

@guarani will the release branch be merged to the trunk once the release is done? Or should I make an additional PR to merge this change to trunk?

@staskus staskus merged commit ac66570 into release/20.9 Oct 10, 2022
@staskus staskus deleted the fix/stats-switching-tabs-not-switching-when-audience-growth-is-showing branch October 10, 2022 17:59
@guarani
Copy link
Contributor

guarani commented Oct 10, 2022

@guarani will the release branch be merged to the trunk once the release is done?

Yep, you don't need to do that.

@twstokes
Copy link
Contributor

Thank you @staskus and @guarani for taking care of this! I was AFK, otherwise I would've been happy to review. 🙇

@mokagio mokagio mentioned this pull request Oct 12, 2022
Merged
3 tasks
@mokagio
Copy link
Contributor

mokagio commented Oct 12, 2022

@staskus this has been bundled as part of 20.9 beta 1 (20.9.0.1).

Thanks for your work 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guarani guarani guarani approved these changes

@twstokes twstokes Awaiting requested review from twstokes

Assignees
No one assigned
Labels
Stats [Type] Bug
Projects
None yet
Milestone
20.9
Development

Successfully merging this pull request may close these issues.
5 participants
@staskus @wpmobilebot @guarani @twstokes @mokagio